Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: impl display for fee and contractaddress structs #279

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

AvivYossef-starkware
Copy link
Contributor

@AvivYossef-starkware AvivYossef-starkware commented Jun 16, 2024

This change is Reviewable

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)

a discussion (no related file):
please add a unit test that makes sure the output is nice-looking... i.e.

fn test_nice() {
    assert_eq!(format("{}", patricia_key!(7_u8)), "0x7");
}


src/transaction.rs line 649 at r1 (raw file):

    Clone,
    Default,
    Display,

why is this needed? it wraps a u128, this should be displayed fine

Code quote:

Display,

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/impl_display_for_structs branch from e87e047 to a02c3bb Compare June 17, 2024 08:44
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

please add a unit test that makes sure the output is nice-looking... i.e.

fn test_nice() {
    assert_eq!(format("{}", patricia_key!(7_u8)), "0x7");
}

Done.



src/transaction.rs line 649 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why is this needed? it wraps a u128, this should be displayed fine

We talked about it later

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/impl_display_for_structs branch from a02c3bb to 4c23cae Compare June 17, 2024 08:46
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)


src/core.rs line 309 at r2 (raw file):

    Ord,
    derive_more:: Deref,
    Display,

why did this move down?

Code quote:

 Display,

src/core.rs line 311 at r2 (raw file):

    Display,
)]
#[display(fmt = "{}", "_0.to_fixed_hex_string()")]

this works? cool

Code quote:

#[display(fmt = "{}", "_0.to_fixed_hex_string()")]

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)


src/core.rs line 309 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why did this move down?

No special reason.


src/core.rs line 311 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this works? cool

yes, its possible if u use the derive_moe:Display macro

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)


src/core.rs line 309 at r2 (raw file):

Previously, AvivYossef-starkware wrote…

No special reason.

please move back

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/impl_display_for_structs branch from 4c23cae to d8b9146 Compare June 17, 2024 10:26
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


src/core.rs line 309 at r2 (raw file):

Previously, dorimedini-starkware wrote…

please move back

I hope that I put it back to the same place

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware merged commit 5565e52 into main Jun 17, 2024
6 checks passed
@AvivYossef-starkware AvivYossef-starkware deleted the aviv/impl_display_for_structs branch June 17, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants